-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: concat of nullable int + bool preserves int dtype #34985
Conversation
if not all( | ||
isinstance(t, _IntegerDtype) | ||
or (isinstance(t, np.dtype) and np.issubdtype(t, np.integer)) | ||
isinstance(t, BaseMaskedDtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry a bit about this. I could easily see other arrays using the BaseMasked stuff (e.g. StringArray) that shouldn't necessarily be considered "integer-like" for this concat.
So I'd be more comfortable with (_IntegerDtype, BooleanDtype)
even though those are synonymous today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with @TomAugspurger here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for BaseMaskedDtype ensures the dtype has a numpy_dtype
, and below we still do a np.find_common_type
on the result. So assuming you have int + string, numpy will return object dtype for that, in which case we still return None from this function (which is equivalent as making the check here more strict and returning None here).
So even when we make StringArray a masked array, this method should already work as expected.
And doing it this way, I don't have to add FloatingDtype to the list of (_IntegerDtype, BooleanDtype)
in the floating PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So even when we make StringArray a masked array, this method should already work as expected.
Good to know.
I don't have to add FloatingDtype to the list of (_IntegerDtype, BooleanDtype) in the floating PR.
Will we want to return None here for float? Or I suppose the find_common_type
stuff will handle that as well, just like string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback does #34985 (comment) make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense, though still feel that there is no downside to being explict about listing the 2 dtypes directy; its more obvious. Agreed that if in the future we add more convertable to integer dtypes they won't automatically be added, but i think that is of lesser benefit that better readability here (i mean you could add a comment, but i think listing the classes is better)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think how it is written now also gives a good message: all BaseMaskedDtype subclasses are supported by this method (which is the case, even though some might return None
from the function later on)
Will we want to return None here for float? Or I suppose the find_common_type stuff will handle that as well, just like string?
Yes, right now we would still return None for float (only if the common numpy dtype is an integer dtype, an EA dtype is returned). Once we have FloatingArray, we would add an additional check for the case that the common numpy dtype is a float dtype, and then return an EA floating dtype.
Thanks. Does the original whatsnew from the other PR describe this change as well? |
I think so yes, it's still in general about preserving EA dtype in concat. But I added the issue number to the exsting whatsnew. |
Merged master. @jreback was the discussion in #34985 (comment) resolved? |
yes this is fine to merge |
Closes #34095